-
Notifications
You must be signed in to change notification settings - Fork 334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix logic for flip, remove useless member variable ( panCache ) #258
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@@ -252,20 +252,38 @@ class Viewport implements IViewport { | |||
vec3.scaleAndAdd(center, center, jVector, (size[1] / 2.0) * spacing[1]); | |||
vec3.scaleAndAdd(center, center, kVector, (size[2] / 2.0) * spacing[2]); | |||
|
|||
const imageToWorld: mat4 = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not imageToWorld (since it does not have Origin in the 4th column). Maybe call it directionMatrix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see what is fixed, the demos work like before, but I wanted the flip to be based on direction which your PR does too
so i tried our own example, it kind of works, but not as expected cd packages/core
yarn run example volumeAPI When I try flippingH on Sagittal it flips but then I can't flip back. I confirm that there was a bug that you couldn't flipH on Sagittal at all, so it is progress I guess? Axial (acquisition)
Sagittal
Coronal
Note: you can try scrolling to a slice other than the middle slice of the volume to see effects of the flip better, you can use cornerstoneTools.utilities.scroll(viewport, {delta: 30}) |
I did some digging into this, I guess the correct implementation should do it based on const { focalPoint, viewPlaneNormal, viewUp } = this.getCamera();
const viewRight = vec3.create();
vec3.cross(viewRight, viewPlaneNormal, viewUp);
const center = focalPoint;
let flipHTx, flipVTx;
const transformToOriginTx = vtkMatrixBuilder
.buildFromRadian()
.identity()
.translate(-center[0], -center[1], -center[2])
.rotateFromDirections(viewRight, [1, 0, 0])
.rotateFromDirections(viewUp, [0, 1, 0]);
const transformBackFromOriginTx = vtkMatrixBuilder
.buildFromRadian()
.identity()
.rotateFromDirections([0, 1, 0], viewUp)
.rotateFromDirections([1, 0, 0], viewRight)
.translate(center[0], center[1], center[2]);
if (flipH) {
this.flipHorizontal = flipHorizontal;
flipHTx = vtkMatrixBuilder
.buildFromRadian()
.multiply(transformToOriginTx.getMatrix())
.scale(-1, 1, 1)
.multiply(transformBackFromOriginTx.getMatrix());
}
if (flipV) {
this.flipVertical = flipVertical;
flipVTx = vtkMatrixBuilder
.buildFromRadian()
.multiply(transformToOriginTx.getMatrix())
.scale(1, -1, 1)
.multiply(transformBackFromOriginTx.getMatrix());
} It works for flipHorizontal for all views (except oblique) but not for vertical. I guess the main issue is that flipping happens at the actor matrix level, and actor matrix is agnostic to the camera view, so that is why some orientations it works and some don't (?) I can dig more later |
there is a feature definition issue : the flip is about flipping the original orientation. this is the current implementation. so if your acquisition is AXIAL, and the view is SAGITAL, horizontal flip, exchanging left and right has no effect on the (sagital) view. Using viewup + normal would be required if you want to achieve an engineer feature : flip view in any orientation. but in this case, there is a need for a large refactor to handle it correctly. for instance you flip image, then using the crosshair you tilt the view, and then if you flip, it's along another |
#271 draft |
can we close this in favor of recent merged PRs? |
I'm closing this, please re-open if needed |
No description provided.